New resource: aws_dlm_lifecycle_policy #5558

tomelliff · 2018-08-15T15:37:04Z

Changes proposed in this pull request:

Adds the aws_dlm_lifecycle_policy resource

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSDlmLifecyclePolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSDlmLifecyclePolicy -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSDlmLifecyclePolicyBasic
--- PASS: TestAccAWSDlmLifecyclePolicyBasic (27.65s)
=== RUN   TestAccAWSDlmLifecyclePolicyFull
--- PASS: TestAccAWSDlmLifecyclePolicyFull (45.55s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	73.213s
...

This PR is currently WIP for a few reasons:

The AWS SDK appears to be failing to parse the timestamp for the created/modified time so falls over in a big heap. I've raised an issue against the SDK in case there's a way to work around it that I'm unaware of but in the meantime I locally patched the SDK in the vendor folder (not pushed) with the diff mentioned in the aforementioned issue.
The resource is failing to read state back from AWS properly for some reason so changes made outside of Terraform don't end up being refreshed back into the state. This is immediately evident with the commented out test for the policy_details.0.schedule.0.create_rule.0.times attribute in the basic test as AWS provides a value if you omit it but this doesn't end up back in the state currently. I'm not sure what I'm doing wrong here because there's nothing in the logs and no panics when refreshing state so would love a pointer here. The heavily nested structure of the API is much more complicated than other resources I've worked on so I guess I've ballsed something up here.
Need to add docs for the resource.
There's a couple of outstanding TODOs on the resource, mostly around validation but also for creating the default service role if it doesn't exist and it's not specified. This could maybe be done in a separate PR but would be a nice thing to do here.

I'll rebase the PR into a single commit after

bflad · 2018-08-16T05:07:20Z

Sorry I don't have time to give in depth review at the moment but to unblock you (hopefully) you can get a better handle on what's wrong with d.Set("policy_details", ...), if return an error from it. 😄

if err := d.Set("policy_details", flattenDlmPolicyDetails(out.Policy.PolicyDetails)); err != nil {
  return fmt.Errorf("error setting policy_details: %s", err)
}

At very quick glance of the flatten functions, here are a few hints:

You'll need to be returning a []map[string]interface{} not just map[string]interface{} to account for the "container" attribute that is a schema.TypeList
You'll need to dereference the API response pointer values (recommended with the SDK functions like aws.StringValue(), aws.Int64Value(), etc.)
You'll want to return []map[string]interface{}{} if the API response object is nil before trying to reference attributes within the object

As an example:

func flattenDlmCreateRule(createRule *dlm.CreateRule) []map[string]interface{} {
 	if createRule == nil {
 	 	return []map[string]interface{}{}
 	}

 	result := make(map[string]interface{})
 	result["interval"] = aws.Int64Value(createRule.Interval)
 	result["internal_unit"] = aws.StringValue(createRule.IntervalUnit)
 	result["times"] = flattenStringList(createRule.Times)

 	return []map[string]interface{}{result}
}

You can also help verify the resource read function by adding import support and using ImportStateVerify testing:

// in resource schema definition
Importer: &schema.ResourceImporter{
	State: schema.ImportStatePassthrough,
},

// adding an acceptance testing TestStep
{
	ResourceName:      resourceName,
	ImportState:       true,
	ImportStateVerify: true,
},

tomelliff · 2018-08-22T18:48:18Z

@bflad thanks for the pointers, think I've got it working now.

I need to rerun the tests (not had a chance to run them since adding the import test) and then squash the mess of commits here.

We also still need to wait for AWS to fix the API response as per the previously linked issue on the AWS SDK before this can be merged.

aws/resource_aws_dlm_lifecycle_policy.go

tomelliff · 2018-08-22T19:05:38Z

aws/resource_aws_dlm_lifecycle_policy.go

+				//	TODO: https://docs.aws.amazon.com/dlm/latest/APIReference/API_LifecyclePolicy.html#dlm-Type-LifecyclePolicy-Description says it has max length of 500 but doesn't mention the regex but SDK and CLI docs only mention the regex and not max length. Check this
+			},
+			"execution_role_arn": {
+				// TODO: Make this not required and if it's not provided then use the default service role, creating it if necessary


Does this want doing in first pass or should it be done later? I'm tempted to do the work for this if AWS take a long time fixing the API response which is blocking this.

I think for starters that we can omit the handling in the code (so people can try this out) and document the solution on the website.

aws/resource_aws_dlm_lifecycle_policy.go

ryan-dyer-sp · 2018-10-08T16:57:34Z

Any update on this?

tomelliff · 2018-10-08T17:50:52Z

I'm still waiting for AWS to fix the response of the API. I've not checked again recently but the linked issue is still open so I'm leaving it until it's closed.

After that I'll fix up the remaining TODOs on this PR and then it should be good for review.

tomelliff · 2018-10-23T07:40:12Z

I've been really busy at work lately so not had any chance to come back and look at the review comments until now and no incentive to do so either with the DLM SDK/API response still being broken (chased AWS about it recently but it's still not fixed).

I've just pushed fixes for all the feedback now and think it's good to go once the DLM API response is fixed (which should be announced in the linked issue or I can respond here when my support ticket is finally closed). When the DLM API response is fixed I'll rebase the branch to fix conflicts and squash the commits.

I've implemented the IntInSlice validation func here but think it probably should go upstream. I'll raise a PR for that over in core and if it gets merged we can remove it from here.

tomelliff · 2018-10-30T11:23:14Z

Just got a reply from AWS support saying the DLM API fix is being deployed across all regions now and they'll let me know when it's fully rolled out. After that I'll test it without my patch to the SDK, rebase the branch to fix merge conflicts and squash the commits.

bflad · 2018-11-05T15:01:13Z

@tomelliff let us know if you need anything!

Fixes hashicorp#5176

tomelliff · 2018-11-05T15:55:34Z

Had a bit of a monster rebase there, let me know if I screwed anything up.

Acceptance test output after rebase:

$ make testacc TESTARGS='-run=TestAccAWSDlmLifecyclePolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSDlmLifecyclePolicy -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSDlmLifecyclePolicyBasic
--- PASS: TestAccAWSDlmLifecyclePolicyBasic (36.54s)
=== RUN   TestAccAWSDlmLifecyclePolicyFull
--- PASS: TestAccAWSDlmLifecyclePolicyFull (52.69s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	(cached)

aws/resource_aws_dlm_lifecycle_policy.go

goochi1 · 2018-11-06T16:20:40Z

has this been merged? cant see it on the site
https://www.terraform.io/docs/providers/aws

bflad

Fantastic work, @tomelliff! Thanks so much for working with AWS on the SDK issue. A few little minor things, but those were easy enough to quickly fix on merge so we can get this released today or tomorrow! 🚀

--- PASS: TestAccAWSDlmLifecyclePolicy_Basic (15.16s)
--- PASS: TestAccAWSDlmLifecyclePolicy_Full (20.28s)

bflad · 2018-11-06T19:59:39Z

aws/resource_aws_dlm_lifecycle_policy.go

+	log.Printf("[INFO] Creating DLM lifecycle policy: %s", input)
+	out, err := conn.CreateLifecyclePolicy(&input)
+	if err != nil {
+		return err


Nit: We should return context about errors so operators and code maintainers can more easily troubleshoot issues, e.g.

return fmt.Errorf("error creating DLM Lifecycle Policy: %s", err)

Same with other similar messaging during Read/Update/Delete. 👍

bflad · 2018-11-06T20:02:11Z

aws/resource_aws_dlm_lifecycle_policy.go

+		PolicyId: aws.String(d.Id()),
+	})
+	if err != nil {
+		if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "ResourceNotFoundException" && !d.IsNewResource() {


Two things here:

We have a helper for handling this type of logic, e.g. isAWSErr(err, dlm.ErrCodeResourceNotFoundException, "")

We should not create a special case during Read after Create that returns an error -- instead if this resource has eventual consistency issues, we should implement a resource.Retry() loop that retries for a minute or two (then can use d.IsNewResource() to determine if its a retryable error).

bflad · 2018-11-06T20:03:44Z

aws/resource_aws_dlm_lifecycle_policy.go

+	})
+	if err != nil {
+		if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "ResourceNotFoundException" && !d.IsNewResource() {
+			d.SetId("")


Nit: When triggering resource recreation, we should return a warning message in the logs so operators and code maintainers can more easily diagnose why it occurred, e.g.

log.Printf("[WARN] DLM Lifecycle Policy (%s) not found, removing from state", d.Id()) d.SetId("")

bflad · 2018-11-06T20:05:52Z

aws/resource_aws_dlm_lifecycle_policy.go

+
+func expandDlmPolicyDetails(cfg []interface{}) *dlm.PolicyDetails {
+	policyDetails := &dlm.PolicyDetails{}
+	m := cfg[0].(map[string]interface{})


To prevent potential panics, we should length and nil check the slice before accessing its first element, e.g.

if len(cfg) == 0 || cfg[0] == nil { return nil } m := cfg[0].(map[string]interface{})

Ack yeah, I keep missing these. Is there a linting rule or something we can use to check for this?

bflad · 2018-11-06T20:07:50Z

aws/resource_aws_dlm_lifecycle_policy.go

+func flattenDlmTags(tags []*dlm.Tag) map[string]string {
+	result := make(map[string]string)
+	for _, t := range tags {
+		result[*t.Key] = *t.Value


To prevent potential panics, we should prefer to use the SDK helpers in this case, e.g.

result[aws.StringValue(t.Key)] = aws.StringValue(t.Value)

bflad · 2018-11-06T20:15:43Z

aws/resource_aws_dlm_lifecycle_policy_test.go

+	return nil
+}
+
+func checkDlmLifecyclePolicyExists(name string) resource.TestCheckFunc {


This function should implement a similar (but opposite) check to the CheckDestroy, where it verifies that the expected resource does actually exist in the API. Admittedly, this is a little buried in the provider development documentation, here: https://www.terraform.io/docs/extend/testing/acceptance-tests/teststep.html#custom-check-functions

return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[name] if !ok { return fmt.Errorf("Not found: %s", name) } if rs.Primary.ID == "" { return fmt.Errorf("Resource %s ID not found", name) } input := dlm.GetLifecyclePolicyInput{ PolicyId: aws.String(rs.Primary.ID), } out, err := conn.GetLifecyclePolicy(&input) if err != nil { return fmt.Errorf("error getting DLM Lifecycle Policy (%s): %s", rs.Primary.ID, err) } return nil }

bflad · 2018-11-06T20:16:41Z

aws/resource_aws_dlm_lifecycle_policy_test.go

+func TestAccAWSDlmLifecyclePolicyBasic(t *testing.T) {
+	resourceName := "aws_dlm_lifecycle_policy.basic"
+
+	resource.Test(t, resource.TestCase{


Nit: This was written before resource.ParallelTest() was available so we'll update these. 👍

bflad · 2018-11-06T20:18:32Z

aws/resource_aws_dlm_lifecycle_policy_test.go

+func dlmLifecyclePolicyBasicConfig() string {
+	return fmt.Sprint(`
+resource "aws_iam_role" "dlm_lifecycle_role" {
+  name = "tf-acc-basic-dlm-lifecycle-role"


Nit: This would've been found with resource.ParallelTest() (had it existed back then!), but resource names like these need to be randomized (check out other testing with rName variables).

bflad · 2018-11-06T20:20:40Z

website/docs/r/dlm_lifecycle_policy.markdown

+---
+layout: "aws"
+page_title: "AWS: aws_dlm_lifecycle_policy"
+sidebar_current: "docs-aws-resource-dlm-lifecycle-policy"


Merge note: Missing sidebar link in website/aws.erb

``` --- PASS: TestAccAWSDlmLifecyclePolicy_Basic (15.16s) --- PASS: TestAccAWSDlmLifecyclePolicy_Full (20.28s) ```

tomelliff · 2018-11-07T14:44:57Z

@bflad thanks for the review, fixes and merge :)

This has been the largest PR I've raised for Terraform and I had a lot of issues with the (unnecessarily in some places IMO) deeply nested API of DLM. I don't get any other opportunities other than PRs to Terraform to write any Go so I learned a ton from this.

bflad · 2018-11-07T20:59:21Z

This has been released in version 1.43.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-02T17:36:39Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

ghost added the size/XL Managed by automation to categorize the size of a PR. label Aug 15, 2018

bflad added new-resource Introduces a new resource. service/dlm Issues and PRs that pertain to the dlm service. labels Aug 16, 2018

ghost added the size/XL Managed by automation to categorize the size of a PR. label Aug 22, 2018

tomelliff commented Aug 22, 2018

View reviewed changes

aws/resource_aws_dlm_lifecycle_policy.go Outdated Show resolved Hide resolved

tomelliff commented Aug 22, 2018

View reviewed changes

aws/resource_aws_dlm_lifecycle_policy.go Outdated Show resolved Hide resolved

tomelliff commented Aug 22, 2018

View reviewed changes

aws/resource_aws_dlm_lifecycle_policy.go Outdated Show resolved Hide resolved

ghost added documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 23, 2018

bflad mentioned this pull request Oct 24, 2018

Automating the Amazon EBS Snapshot Lifecycle through Terraform #6255

Closed

tomelliff force-pushed the dlm-lifecycle-policy-resource branch from 99ba014 to e2cfcb8 Compare November 5, 2018 15:50

ghost added the provider Pertains to the provider itself, rather than any interaction with AWS. label Nov 5, 2018

tomelliff force-pushed the dlm-lifecycle-policy-resource branch from e2cfcb8 to 28981d6 Compare November 5, 2018 15:53

Add DLM Lifecycle Policy resource

3eb1360

Fixes hashicorp#5176

tomelliff force-pushed the dlm-lifecycle-policy-resource branch from 28981d6 to 3eb1360 Compare November 5, 2018 15:55

tomelliff changed the title ~~WIP: New resource: aws_dlm_lifecycle_policy~~ New resource: aws_dlm_lifecycle_policy Nov 5, 2018

chenrui333 reviewed Nov 5, 2018

View reviewed changes

aws/resource_aws_dlm_lifecycle_policy.go Show resolved Hide resolved

tomelliff mentioned this pull request Nov 5, 2018

DLM/GetLifecyclePolicy fails to parse created/modified timestamps aws/aws-sdk-go#2100

Closed

bflad added this to the v1.43.0 milestone Nov 6, 2018

bflad approved these changes Nov 6, 2018

View reviewed changes

bflad merged commit 3eb1360 into hashicorp:master Nov 6, 2018

bflad added a commit that referenced this pull request Nov 6, 2018

resource/aws_dlm_lifecycle_policy: Address #5558 PR feedback

5185e50

``` --- PASS: TestAccAWSDlmLifecyclePolicy_Basic (15.16s) --- PASS: TestAccAWSDlmLifecyclePolicy_Full (20.28s) ```

bflad added a commit that referenced this pull request Nov 6, 2018

Update CHANGELOG for #5558

1040c9f

mdlavin mentioned this pull request Nov 21, 2018

V1.46.0 patched lifeomic mdlavin/terraform-provider-aws#3

Closed

ghost locked and limited conversation to collaborators Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resource: aws_dlm_lifecycle_policy #5558

New resource: aws_dlm_lifecycle_policy #5558

tomelliff commented Aug 15, 2018

bflad commented Aug 16, 2018

tomelliff commented Aug 22, 2018

tomelliff Aug 22, 2018

bflad Aug 22, 2018

ryan-dyer-sp commented Oct 8, 2018

tomelliff commented Oct 8, 2018

tomelliff commented Oct 23, 2018 •

edited

Loading

tomelliff commented Oct 30, 2018

bflad commented Nov 5, 2018

tomelliff commented Nov 5, 2018

goochi1 commented Nov 6, 2018

bflad left a comment

bflad Nov 6, 2018

bflad Nov 6, 2018

bflad Nov 6, 2018

bflad Nov 6, 2018

tomelliff Nov 7, 2018

bflad Nov 6, 2018

bflad Nov 6, 2018

bflad Nov 6, 2018

bflad Nov 6, 2018

bflad Nov 6, 2018

tomelliff commented Nov 7, 2018

bflad commented Nov 7, 2018

ghost commented Apr 2, 2020

New resource: aws_dlm_lifecycle_policy #5558

New resource: aws_dlm_lifecycle_policy #5558

Conversation

tomelliff commented Aug 15, 2018

bflad commented Aug 16, 2018

tomelliff commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryan-dyer-sp commented Oct 8, 2018

tomelliff commented Oct 8, 2018

tomelliff commented Oct 23, 2018 • edited Loading

tomelliff commented Oct 30, 2018

bflad commented Nov 5, 2018

tomelliff commented Nov 5, 2018

goochi1 commented Nov 6, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomelliff commented Nov 7, 2018

bflad commented Nov 7, 2018

ghost commented Apr 2, 2020

tomelliff commented Oct 23, 2018 •

edited

Loading